Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for sqlite3_unlock_notify #439

Merged
merged 2 commits into from
Oct 21, 2018
Merged

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Jul 14, 2017

For go-gitea/gitea#2040

Added new build tag sqlite_unlock_notify.

name      old req/s   new req/s   delta            
Exec       219k ±20%   217k ±18%     ~     (p=0.690 n=5+5)                                            
Query     75.8k ±12%  67.9k ±13%     ~     (p=0.095 n=5+5)                                            
Params    74.1k ± 3%  68.5k ± 6%   -7.63%  (p=0.008 n=5+5)                                            
Stmt       119k ± 1%   108k ± 5%   -9.56%  (p=0.008 n=5+5)                                            
Rows      2.21k ± 1%  2.08k ± 1%   -5.61%  (p=0.016 n=5+4)                                            
StmtRows  2.30k ± 2%  2.05k ±11%  -10.78%  (p=0.008 n=5+5)  

@mattn
Copy link
Owner

mattn commented Aug 30, 2017

kick travis

@mattn mattn closed this Aug 30, 2017
@mattn mattn reopened this Aug 30, 2017
@mattn
Copy link
Owner

mattn commented Aug 30, 2017

Could you please rebase ?

@typeless typeless force-pushed the add-unlock-notify branch 3 times, most recently from 708b649 to 39f6e5d Compare September 4, 2017 07:43
@coveralls
Copy link

coveralls commented Sep 4, 2017

Coverage Status

Coverage remained the same at 58.344% when pulling 68e53de on typeless:add-unlock-notify into c67b489 on mattn:master.

@typeless
Copy link
Contributor Author

typeless commented Sep 6, 2017

I'd like to add a test for what the PR tries to fix but cannot deliver it in the first few attempts. I'll try later when I have more free time.

@gjrtimmer
Copy link
Collaborator

@typeless Could you rebase

@typeless
Copy link
Contributor Author

@gjrtimmer Done

@mattn
Copy link
Owner

mattn commented May 30, 2018

I wonder this can be tested.

Copy link
Collaborator

@gjrtimmer gjrtimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think only one tiny code comment

sqlite3.go Outdated Show resolved Hide resolved
@gjrtimmer
Copy link
Collaborator

@mattn I think you are right we need some code to test it, maybe even an example.

@typeless Instead of currently trying to write a test for it, You did probably design this with some code to see if it worked. Could you add an example for this to the _example folder ?

@gjrtimmer gjrtimmer mentioned this pull request Jul 20, 2018
@bvinc
Copy link

bvinc commented Sep 4, 2018

I notice that in the step function, you check the extended error code to make sure it's SQLITE_LOCKED_SHAREDCACHE, but you don't do this in the prepare code. Why not?

@typeless
Copy link
Contributor Author

typeless commented Sep 4, 2018

@bvinc It was modified from the example code: https://sqlite.org/unlock_notify.html

The explanation of the SQLITE_LOCKED_SHAREDCACHE check is described in the last few paragraphs.
It seems to me that sqlite3_blocking_prepare_v2 would not result in the same scenario as qlite3_blocking_step does.

@typeless
Copy link
Contributor Author

typeless commented Sep 4, 2018

By the way, I have found a test which can reproduce the "database is locked" error but it turned out that the PR does not fix the problem. I'll try to figure it out later and see if this PR has any hope to be helpful.

@bvinc
Copy link

bvinc commented Sep 17, 2018

Which test can still reproduce "database is locked" error with this PR?

@bvinc
Copy link

bvinc commented Sep 17, 2018

I found your test case in a closed PR. I assume this is the test case that you're talking about. I believe that there is some confusion again between SQLITE_BUSY and SQLITE_LOCKED.

Without using shared cache mode (which applies to your test), when two transactions conflict, the one that fails uses the busy handler, which waits a small number of random milliseconds, and then tries again. Once this fails for 5 seconds (which is the default busy timeout for this driver), then it fails with SQLITE_BUSY, which says database is locked. What's happening with your test case is that one statement is spinning so fast that it's completely starving out the other statement for a full 5 seconds. This is completely normal and just how SQLite works. You can see that if you increase the busy timeout by adding "?_busy_timeout=60000" that it won't give an error anymore.

But this PR is about something completely different. This PR is about stopping people from getting SQLITE_LOCKED when using shared cache mode (which your test case did not use).

@typeless
Copy link
Contributor Author

@bvinc You're correct I misunderstood the two different errors. I need to try it again when I come back to this.

@typeless
Copy link
Contributor Author

@mattn @gjrtimmer @bvinc The test has been added. PTAL.

@typeless typeless changed the title [WIP] Add support for sqlite3_unlock_notify Add support for sqlite3_unlock_notify Sep 30, 2018
@typeless
Copy link
Contributor Author

Hmmm, I have to make it compatible with USE_LIBSQLUTE3 .

@typeless typeless force-pushed the add-unlock-notify branch 3 times, most recently from ad29852 to 6903ee1 Compare September 30, 2018 08:24
@typeless
Copy link
Contributor Author

A new build tag sqlite_unlock_notify has been introduced.

@typeless
Copy link
Contributor Author

typeless commented Oct 1, 2018

Found a bug while trying to integrate the change into Gitea. I am looking into it.

@typeless typeless changed the title Add support for sqlite3_unlock_notify [WIP] Add support for sqlite3_unlock_notify Oct 4, 2018
@typeless typeless force-pushed the add-unlock-notify branch 3 times, most recently from 3da01bd to 5cc988d Compare October 20, 2018 01:55
@typeless typeless changed the title [WIP] Add support for sqlite3_unlock_notify Add support for sqlite3_unlock_notify Oct 20, 2018
@typeless
Copy link
Contributor Author

Found a bug while trying to integrate the change into Gitea. I am looking into it.

The bugs have been figured out. For now, all available tests pass.

@mattn
Copy link
Owner

mattn commented Oct 20, 2018

Thank you. Sounds good to me. One thing. please add _ for the functions always to be not use same namespace sqlite_.

@typeless
Copy link
Contributor Author

@mattn Done. Is that what you mean?

@mattn
Copy link
Owner

mattn commented Oct 21, 2018

Yes! Thank you.

@mattn mattn merged commit f3aa5ce into mattn:master Oct 21, 2018
lstoll added a commit to lstoll/wherewasi that referenced this pull request Nov 15, 2020
https://sqlite.org/unlock_notify.html
mattn/go-sqlite3#439

Rather than erroring when a table is locked, this should hopefully
make the action just wait
lstoll added a commit to lstoll/wherewasi that referenced this pull request Nov 15, 2020
* Log error details when persisting a location

* Flag in sqlite3_unlock_notify

https://sqlite.org/unlock_notify.html
mattn/go-sqlite3#439

Rather than erroring when a table is locked, this should hopefully
make the action just wait
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants